[#2869] feat(core): Add the core logic of authorization #3050

qqqttt123 · 2024-04-19T13:00:01Z

What changes were proposed in this pull request?

Add the core logic of authorization.

Why are the changes needed?

Fix: #2869

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add new uts.

qqqttt123 · 2024-04-19T17:18:10Z

@jerryshao This pull request is ready to review.

qqqttt123 · 2024-04-20T03:29:35Z

@jerryshao I have removed version filter.

jerryshao · 2024-04-20T03:39:29Z

server/src/main/java/com/datastrato/gravitino/server/web/rest/GroupOperations.java

+                  return Utils.ok(
+                      new GroupResponse(
+                          DTOConverters.toDTO(accessControlManager.getGroup(metalake, group))));
+                });


Why do you need to wrap these two operations into one tree lock?

jerryshao · 2024-04-20T03:45:32Z

server/src/main/java/com/datastrato/gravitino/server/web/rest/GroupOperations.java

+                identifier,
+                LockType.READ,
+                () -> {
+                  AuthorizationUtils.checkIsMetalakeOwner(identifier);


Why does the creator only has the permission to do this operation? What if the owner is not available any more?

I think you should have a different role for metalake admins to have the metalake operations.

By referring other systems, you should have a series of role and privileges for admin users, like create metalake, access metalake, create/drop role/user/groups. Otherwise, your current solution has lots of limitations.

jerryshao · 2024-04-20T03:50:48Z

server/src/main/java/com/datastrato/gravitino/server/web/rest/MetalakeAdminOperations.java

-                      DTOConverters.toDTO(
-                          accessControlManager.addMetalakeAdmin(request.getName())))));
+          () -> {
+            AuthorizationUtils.checkIsServiceAdmin();


Why don't you do this in a filter, not just adding it one by one interface?

jerryshao · 2024-04-20T03:56:47Z

server/src/main/java/com/datastrato/gravitino/server/web/rest/MetalakeOperations.java

+                        // Filter the metalakes which aren't created by the user
+                        List<Metalake> filteredMetalakes = Lists.newArrayList();
+                        for (Metalake metalake : orignMetalakes) {
+                          if (metalake.auditInfo().creator().equals(currentUser)) {


Also here, why only the creator has permission to list metalakes, what if the creator is leaving, how to handle this scenario?

qqqttt123 · 2024-04-23T07:32:08Z

@jerryshao Could you help me review this pull request?

jerryshao · 2024-04-23T08:30:54Z

api/src/main/java/com/datastrato/gravitino/authorization/Privileges.java

+
+    @Override
+    public boolean supportsSecurableObjectType(SecurableObject.Type type) {
+      return true;


Can you please add some comment here andy below for grant privilege?

jerryshao · 2024-04-23T08:50:41Z

api/src/main/java/com/datastrato/gravitino/authorization/SecurableObjects.java

-   * object is only used for metalake admin. You can't grant any privilege to this securable object.
-   * You can't bind this securable object to any role, too.
+   * object is reserved for system. You can't grant any privilege to this securable object. You
+   * can't bind this securable object to any role, too.


There you mentioned that "You can give the securable object the privileges CREATE METALAKE", but here you mentioned that "You can't grant any privilege to this securable object". I think these two sentences are self-contradictory, you should have a more accurate description.

Besides, why you can't bind this securable object to a role?

Yes, for users, users can't grant any privilege to allMetalakes. But system pre-created a role binds allMetalakes. AllMetalakes isn't used for the normal user.

We shouldn't allow user to use allMetalakes. It can inflluence the privileges of all metalakes.It's too dangerous.

jerryshao · 2024-04-23T08:58:28Z

core/src/main/java/com/datastrato/gravitino/authorization/UserGroupManager.java

@@ -48,8 +48,18 @@ class UserGroupManager {
  }

  User addUser(String metalake, String name) throws UserAlreadyExistsException {
+
+    AuthorizationUtils.checkMetalakeExists(metalake);
+    AuthorizationUtils.checkPermission(


It's not a best practice to checkPermission everywhere in this package, why can't you do this in a filter before the request actually executed?

The implementation here is so broken and split into two groups of API, which makes the change so hard to maintain.

It's not a best practice to checkPermission everywhere in this package, why can't you do this in a filter before the request actually executed?

Some operations will execute some extra actions after the operations are executed For example, list operation. We should filter the entities which we don't have any privilege.
For example, create operation. We should create some system roles for these operations.

It's due to your design model problem, you're not implementing a consistent role base model, which makes the code so broken, in which you differentiate service user, admin user and normal user, each user has a different implementation logic to handle.

Anyway, I don't like the current implementation, it is not a good code implementation.

jerryshao · 2024-04-23T09:12:55Z

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

+    }
+  }
+
+  public static boolean isNotAllowedToList(


This method name is so weird.

jerryshao · 2024-04-23T09:14:26Z

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

+    }
+  }
+
+  public static boolean isNotAllowedForAnyPrivileges(


Why do you need to name the method to "isNotXXX", it's no so intuitive.

I usually use !xxxxx. IDEA suggest me use isNotxxx to replace it.

jerryshao · 2024-04-23T10:18:28Z

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

@@ -57,16 +49,195 @@ public static NameIdentifier ofUser(String metalake, String user) {
        metalake, Entity.SYSTEM_CATALOG_RESERVED_NAME, Entity.USER_SCHEMA_NAME, user);
  }

+  public static NameIdentifier ofMetalakeAdminRole() {


metalake admin is not a role, it is a user with some predefined roles.

core/src/main/java/com/datastrato/gravitino/Entity.java

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

yuqi1129 · 2024-06-13T10:14:15Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

-      return userEntity;
+      if (isServiceAdmin(user)) {
+        if (hasUserMetalakeCreateRole(user)) {
+          throw new EntityAlreadyExistsException(


I don't think you shoud throw EntityAlreadyExistsException, maybe you need need to throw PrivilegesAlreadyGrantedException or something similar.

I think so.

yuqi1129 · 2024-06-13T10:16:01Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

-      store.put(userEntity, false /* overwritten */);
-      return userEntity;
+      if (isServiceAdmin(user)) {
+        if (hasUserMetalakeCreateRole(user)) {


hasCreateUserMetalakeRole may be more clear.

Changed to hasCreateMetalakeRole

yuqi1129 · 2024-06-13T11:49:27Z

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

+          }
+        }
+      }
+    } catch (NoSuchUserException noSuchUserException) {


When will Gravitino throw this kind of exception?

I will add some comments for this.

I see, Maybe we can narrow the try/catch block by the following code.

List<RoleEntity> roles; try { roles = accessControlManager.listRolesByUser(metalake, currentUser); } catch( XXXX) { return false; } ... doCheckPermissin() ...

You may need to add the following comments.

Any privilege is satisfied for the object privileges to return true.

We only check the allowed privileges and only the user have the allow privileges can do some operations, we will not check if a user has the denied privileges.

yuqi1129 · 2024-06-13T12:01:17Z

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

+      for (RoleEntity role : roles) {
+        for (Privilege privilege : object.privileges()) {
+          if (role.hasPrivilegeWithCondition(object, privilege.name(), Privilege.Condition.DENY)) {
+            continue;
+          }
+
+          if (role.hasPrivilegeWithCondition(object, privilege.name(), Privilege.Condition.ALLOW)) {
+            return true;
+          }


I don't understand this block, assuming a role has this privileges: "deny on metalake about create table" and the SecurableObject is "deny on a schema of the metalake about create table", In my opinion, satisfyPrivileges should return true.

I will add some comments for this.

core/src/main/java/com/datastrato/gravitino/Entity.java

yuqi1129 · 2024-06-25T07:30:08Z

core/src/main/java/com/datastrato/gravitino/Entity.java

+   */
+  String SYSTEM_METALAKE_MANAGE_USER_ROLE = "system_role_metalake_system_manage_user";
+
+  /** The reserved the prefix of the role names */


The reserved role name's prefix.

Changed to The reserved role names' prefix.

core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

yuqi1129 · 2024-06-25T07:53:07Z

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

+        }
+      }
+    } catch (NoSuchUserException noSuchUserException) {
+      // If one user doesn't exist in the metalake, the user doesn't have any privilege.


I think this is ok.

yuqi1129 · 2024-06-25T08:08:20Z

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

+            continue;
+          }
+
+          if (role.hasPrivilegeWithCondition(object, privilege.name(), Privilege.Condition.ALLOW)) {


Considering the following case:
Privileges of object is: Create table: allow, create database: allow
Privileges of current user: create table: allow, create table: deny

So the final result is true????

yuqi1129 · 2024-06-25T08:18:54Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

    try {
-      store.put(userEntity, false /* overwritten */);
-      return userEntity;
+      if (isServiceAdmin(user)) {


Since the user is a service admin, do we still need to add roles SYSTEM_METALAKE_MANAGE_USER_ROLE and METALAKE_CREATE_ROLE to it?

Yes, because we used the role to execute authorization operation.

jerryshao

I just did a cursory review, I found that you have many changes in authorization package, but looks like you doesn't have many UTs to cover all the changes. I would suggest you add more tests to cover the changes you made.

jerryshao · 2024-06-25T11:20:22Z

core/src/main/java/com/datastrato/gravitino/Entity.java

+  String SYSTEM_METALAKE_MANAGE_USER_ROLE = "system_role_metalake_system_manage_user";
+
+  /** The reserved the prefix of the role names */
+  String SYSTEM_RESERVED_ROLE_NAME_PREFIX = "system_role";


I think it would better to move these definitions to Role.

I moved to Role.

jerryshao · 2024-06-25T11:22:19Z

core/src/main/java/com/datastrato/gravitino/GravitinoEnv.java

+    NameIdentifier systemMetalake = NameIdentifier.of(Entity.SYSTEM_METALAKE_RESERVED_NAME);
+    if (!metalakeManager.metalakeExists(systemMetalake)) {
+      metalakeManager.createMetalake(systemMetalake, null, Collections.emptyMap());
+    }


I think we'd better have a new method to do the system-level metalake, catalog and other creations.

Because AccessManager need system metalake. I extract a method in this class.
For system catalog and other creation, I can create a new class to handle them.

jerryshao · 2024-06-25T11:23:10Z

core/src/main/java/com/datastrato/gravitino/authorization/AccessControlManager.java

@@ -294,6 +286,17 @@ public boolean deleteRole(String metalake, String role) throws NoSuchMetalakeExc
    return doWithNonAdminLock(() -> roleManager.deleteRole(metalake, role));
  }

+  public List<RoleEntity> getRolesByUserFromMetalake(String metalake, String currentUser) {


listRolesByUser

jerryshao · 2024-06-25T11:27:15Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

+        return true;
+      } else {
+        return false;
+      }


You should clearly define the behavior of false. in Gravitino, false only means not existed, here is it suitable to return false?

If the role which creates the metalake doesn't exist, we return will false.

jerryshao · 2024-06-25T12:27:09Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

-      return userEntity;
+      if (isServiceAdmin(user)) {
+        if (hasUserMetalakeCreateRole(user)) {
+          throw new EntityAlreadyExistsException(


I think so.

jerryshao · 2024-06-25T12:31:03Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

-      LOG.warn("User {} in the metalake admin already exists", user, e);
-      throw new UserAlreadyExistsException("User %s in the metalake admin already exists", user);
+      LOG.warn("The metalake admin {} has been added before", user, e);
+      throw new RoleAlreadyExistsException("The metalake admin %s has been added before", user);


Why do you throw "EntityAlreadyExistsException" and convert to "RoleAlreadyExistsException", why don't you directly throw "RoleAlreadyExistsException"?

jerryshao · 2024-06-25T12:36:11Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

+    return AuthorizationUtils.ofUser(Entity.SYSTEM_METALAKE_RESERVED_NAME, user);
+  }
+
+  private boolean hasCreateMetalakeRole(String user) throws IOException {


hasMetakeCreateRole?

jerryshao · 2024-06-25T12:37:28Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

+              Entity.EntityType.USER);
+
+      for (UserEntity userEntity : userEntities) {
+        // Case 1: If the user is the service admin, we should the system metalake manage user role


"we should" what? Please fix the grammar issue.

jerryshao · 2024-06-25T12:56:20Z

server/src/main/java/com/datastrato/gravitino/server/web/filter/MetalakeAdminFilter.java

+      privileges.add(Privileges.RemoveUser.allow());
+    }
+
+    return SecurableObjects.ofMetalake(Entity.SYSTEM_METALAKE_RESERVED_NAME, privileges);


Why it uses "system" metalake name, not "*"?

I don't understand why we need to create a system metalake object with user privilege to check the permission. Please explain more here.

I will add the comment

System metalake will store the privileges which don't belong any concrete metalake. `*` means all the metalakes. We don't have a concrete metalake named `*`. So we use system metlake to store the privileges about `*`.

jerryshao · 2024-06-25T13:06:29Z

core/src/main/java/com/datastrato/gravitino/meta/RoleEntity.java

@@ -155,6 +162,23 @@ public Namespace namespace() {
    return namespace;
  }

+  public boolean hasPrivilegeWithCondition(


I would suggest not put privilege checking logic in this object class, you'd create a new method class to implement such logic.

I moved the logic to AuthorizationUtils.

jerqi · 2024-06-26T07:50:56Z

I just did a cursory review, I found that you have many changes in authorization package, but looks like you doesn't have many UTs to cover all the changes. I would suggest you add more tests to cover the changes you made.

I can add some new uts for AuthorizationUtils.satisfyPrivileges. In my origin thought, I want to cover the cases in the uts and ITS of the filter.

yuqi1129 · 2024-06-26T12:39:36Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

@@ -76,7 +93,16 @@ User addMetalakeAdmin(String user) {

  boolean removeMetalakeAdmin(String user) {


removeUserFromMetalakeAdmin?

I don't think this is a good name.

yuqi1129 · 2024-06-26T12:41:16Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

+        return store.delete(ofMetalakeAdmin(user), Entity.EntityType.USER);
+      }
+
+      if (hasMetalakeCreateRole(user)) {


So, if the code goes here, the user is the service admin. Am I right? Why do we need to judge whether the user has the role MetalakeCreateRole?

I return the correct value whether the MetalakeCreateRole is removed successfully.

yuqi1129 · 2024-06-26T12:45:32Z

core/src/main/java/com/datastrato/gravitino/authorization/AdminManager.java

+
+      for (UserEntity userEntity : userEntities) {
+        // Case 1: If the user is the service admin, we should add the system metalake manage user
+        // role


Format the comments.

yuqi1129 · 2024-06-26T12:52:05Z

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

+          }
+        }
+      }
+    } catch (NoSuchUserException noSuchUserException) {


I see, Maybe we can narrow the try/catch block by the following code.

List<RoleEntity> roles; try { roles = accessControlManager.listRolesByUser(metalake, currentUser); } catch( XXXX) { return false; } ... doCheckPermissin() ...

yuqi1129 · 2024-06-26T13:00:19Z

core/src/main/java/com/datastrato/gravitino/authorization/AuthorizationUtils.java

+          }
+        }
+      }
+    } catch (NoSuchUserException noSuchUserException) {


You may need to add the following comments.

Any privilege is satisfied for the object privileges to return true.

We only check the allowed privileges and only the user have the allow privileges can do some operations, we will not check if a user has the denied privileges.

qqqttt123 force-pushed the ISSUE-2869-4 branch 3 times, most recently from 53f1324 to 2574f78 Compare April 19, 2024 16:30

qqqttt123 self-assigned this Apr 19, 2024

qqqttt123 requested a review from jerryshao April 19, 2024 16:42

jerryshao reviewed Apr 20, 2024

View reviewed changes

qqqttt123 force-pushed the ISSUE-2869-4 branch from a9137b0 to 1f557e3 Compare April 22, 2024 16:22

qqqttt123 requested a review from jerryshao April 23, 2024 07:28

jerryshao reviewed Apr 23, 2024

View reviewed changes

Heng Qin added 3 commits May 22, 2024 11:45

Supports multiple securable objects

85c56aa

Remove limit

2dab53a

Remove blank

f97d872

jerqi force-pushed the ISSUE-2869-4 branch from 58479d0 to 49f227c Compare May 22, 2024 08:31

Heng Qin added 4 commits May 22, 2024 19:10

Update SQL script

4ebe2ce

Modify document and add upgrade script

f8d0649

Add new uts

65108ca

fix style

d71132f

jerqi force-pushed the ISSUE-2869-4 branch 2 times, most recently from ba4e52a to 33c0244 Compare May 23, 2024 12:54

Heng Qin added 5 commits May 24, 2024 10:09

Change type

68a2cd4

Add fields

1487d89

Use another table to store securable objects

927ec29

Merge remote-tracking branch 'upstream/main' into ISSUE-3343

80a3335

Commit information

dd237ba

yuqi1129 reviewed Jun 13, 2024

View reviewed changes

jerqi added 7 commits June 19, 2024 20:48

Address comments

3426521

Merge remote-tracking branch 'upstream/main' into ISSUE-2869-4

a522cde

fix style

280d716

fix ut

bbfbb11

address comments

6463b3f

Merge remote-tracking branch 'upstream/main' into ISSUE-2869-4

bd240ce

fix conflicts

a3ad730

yuqi1129 reviewed Jun 25, 2024

View reviewed changes

jerryshao reviewed Jun 25, 2024

View reviewed changes

address comments

319a91f

jerqi added 4 commits June 26, 2024 19:40

Add ut

f39f262

Remove todo

c23924a

fix style

3dc29ed

fix ut

940ea14

yuqi1129 reviewed Jun 26, 2024

View reviewed changes

jerqi added 3 commits June 27, 2024 17:21

address comments

31a3a2b

refactor another class

a714cdd

fix ut

b7c30fa

jerqi force-pushed the ISSUE-2869-4 branch from 443e092 to 34e27b0 Compare June 27, 2024 12:24

qqqttt123 changed the title ~~[#2869] feat(server): Add the authorization for admins~~ [#2869] feat(server): Add the PermissionChcker Jun 27, 2024

qqqttt123 changed the title ~~[#2869] feat(server): Add the PermissionChcker~~ [#2869] feat(core): Add the PermissionChcker Jun 27, 2024

jerqi force-pushed the ISSUE-2869-4 branch from 34e27b0 to 2eb092d Compare June 27, 2024 12:27

qqqttt123 changed the title ~~[#2869] feat(core): Add the PermissionChcker~~ [#2869] feat(core): Add the core logic of authorization Jun 27, 2024

Remove admin filter

bd54c41

jerqi force-pushed the ISSUE-2869-4 branch from 2eb092d to bd54c41 Compare June 27, 2024 12:32

		@@ -76,7 +93,16 @@ User addMetalakeAdmin(String user) {

		boolean removeMetalakeAdmin(String user) {

[#2869] feat(core): Add the core logic of authorization #3050

Are you sure you want to change the base?

[#2869] feat(core): Add the core logic of authorization #3050

Conversation

qqqttt123 commented Apr 19, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

qqqttt123 commented Apr 19, 2024

qqqttt123 commented Apr 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao Apr 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi commented Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qqqttt123 commented Apr 19, 2024 •

edited

Loading

jerryshao Apr 20, 2024 •

edited

Loading

jerqi Jun 26, 2024 •

edited

Loading

jerqi commented Jun 26, 2024 •

edited

Loading

jerqi Jun 27, 2024 •

edited

Loading